Skip to content

Specify default security user provider. #1132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 8, 2020
Merged

Specify default security user provider. #1132

merged 1 commit into from
Jul 8, 2020

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Jul 7, 2020

Hello,

I published a Symfony bundle ecphp/cas-bundle, that bundle has a Symfony flex recipe.

When users are trying the bundle against the Symfony demo (symfony new project --demo), it fails during installation because the flex recipe copies configuration files from the bundle into the app config directory.
Among the copied files, there is this one in particular, which contains a new security user provider.

The demo app has a firewall already configured to use the only existing provider: database_users.

When installing the demo app and the ecphp/cas-bundle bundle, it fails because multiple user providers exists in the application and the firewall do not know which one to use for the form_login listener.

My proposal is to explicitly specify which user provider to use for the form_login listener so we don't have this issue anymore with bundles providing their own security user provider.

When modifying the security.yaml file before running the composer require ecphp/cas-bundle, it works without any issue.

Log

$ symfony new test-ecas --demo
* Creating a new Symfony Demo project with Composer
  (running /home/pol/bin/composer create-project symfony/symfony-demo /home/pol/dev/git/symfony-demo/test-ecas)

* Setting up the project under Git version control
  (running git init /home/pol/dev/git/symfony-demo/test-ecas)
                                                                                                                        
 [OK] Your project is now ready in /home/pol/dev/git/symfony-demo/test-ecas                                                   
 
 $ cd test-ecas/
$ composer require ecphp/cas-bundle
Using version ^2.1 for ecphp/cas-bundle
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Restricting packages listed in "symfony/symfony" to "5.1.*"
Package operations: 11 installs, 0 updates, 0 removals
  - Installing psr/http-message (1.0.1): Loading from cache
  - Installing symfony/psr-http-message-bridge (v2.0.1): Loading from cache
  - Installing symfony/http-client-contracts (v2.1.3): Loading from cache
  - Installing symfony/http-client (v5.1.2): Loading from cache
  - Installing psr/http-factory (1.0.1): Loading from cache
  - Installing php-http/message-factory (v1.0.2): Loading from cache
  - Installing nyholm/psr7 (1.3.0): Loading from cache
  - Installing psr/http-client (1.0.1): Loading from cache
  - Installing league/uri-query-parser (1.0.1): Loading from cache
  - Installing ecphp/cas-lib (1.0.5): Loading from cache
  - Installing ecphp/cas-bundle (2.1.0): Loading from cache
Package zendframework/zend-code is abandoned, you should avoid using it. Use laminas/laminas-code instead.
Package zendframework/zend-eventmanager is abandoned, you should avoid using it. Use laminas/laminas-eventmanager instead.
Writing lock file
Generating autoload files
70 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
Symfony operations: 2 recipes (dd529a900427cb02dbeddf6e23d4992a)
  - Configuring nyholm/psr7 (>=1.0): From github.com/symfony/recipes:master
  - Configuring ecphp/cas-bundle (>=2.0): From github.com/symfony/recipes-contrib:master
ocramius/package-versions:  Generating version class...
ocramius/package-versions: ...done generating version class
Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!
!!  In SecurityExtension.php line 626:
!!
!!    Not configuring explicitly the provider for the "form_login" listener on "m
!!    ain" firewall is ambiguous as there is more than one registered provider.
!!
!!
!!
Script @auto-scripts was called via post-update-cmd

Installation failed, reverting ./composer.json to its original content.
$

@drupol drupol changed the title Specify default user provider. Specify default security user provider. Jul 7, 2020
@javiereguiluz
Copy link
Member

Thanks for this proposal Pol ... I don't know much about this (@wouterj knows everything about the new security system) but I think this proposal makes sense (I even think that Symfony made it "mandatory" a while ago ... or maybe it was a related change, I can't remember exactly).

In any case, I like the overall idea ... and I like it even more because it solves your issue. But let's wait for more comments to see if there's some technical drawback. Thanks!

@drupol
Copy link
Contributor Author

drupol commented Jul 7, 2020

Waiting for @wouterj 's feedback.

wouterj
wouterj approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The implicit default is nice for quick bootstrapping, but it makes more sense to explicitly specify the user providers for each authentication provider. The MakerBundle was also recently updated to always specify the user provider, so I think it makes sense to update the demo app as well.

I think it might make more sense to have this option as last in the list instead of first (the first ones are specific for the form_login).

Btw, I think you're talking about the entry_point being mandatory in the new security system, Javier. However, the entry_point DX when using the old system is quite bad - so we should not add it atm (demo is not using the new system yet).

@drupol
Copy link
Contributor Author

drupol commented Jul 7, 2020

Going to make the changes now.

@drupol
Copy link
Contributor Author

drupol commented Jul 7, 2020

@javiereguiluz and @wouterj Ready for review again now.

@drupol
Copy link
Contributor Author

drupol commented Jul 8, 2020

Do you think we can move this forward now @javiereguiluz ?

@wouterj
Copy link
Member

wouterj commented Jul 8, 2020

It's barely 23 hours since the last comment on this PR. There is a lot more to do in the life of open source maintainers than merging PRs in one of the 10+ packages they maintain :)

@drupol
Copy link
Contributor Author

drupol commented Jul 8, 2020

That is true as well :-) Didn't meant to push, sorry ✌️

@javiereguiluz
Copy link
Member

And this is now merge 🎉 Thanks Pol for the contribution and thanks Wouter for the quick review!

@javiereguiluz javiereguiluz merged commit 1d809c2 into symfony:master Jul 8, 2020
@drupol drupol deleted the specify-default-user-provider branch July 8, 2020 15:16
@drupol
Copy link
Contributor Author

drupol commented Jul 8, 2020

Merci, hope you don't feel offended by my previous message!

@javiereguiluz
Copy link
Member

Of course not!!! We usually merge things fast ... but these days I'm a bit busy with work-related stuff. But I completely understand the excitement to get some contribution merged fast (I feel the same all the time with my contributions 😊 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drupol @javiereguiluz @wouterj